-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[chore] Create RFC for Optional config types #12596
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
[chore] Create RFC for Optional config types #12596
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #12596 +/- ##
=======================================
Coverage 91.43% 91.43%
=======================================
Files 487 488 +1
Lines 26808 26879 +71
=======================================
+ Hits 24511 24577 +66
- Misses 1814 1818 +4
- Partials 483 484 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
@yurishkuro could you take a look at this? We're taking a closer look at whether we want this type for 1.0 and want to make sure if we add it that it will cover all config use cases and won't have any major downsides. I would appreciate your input here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
Co-authored-by: Pablo Baeyens <[email protected]>
Co-authored-by: Pablo Baeyens <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can then remove https://github.com/open-telemetry/opentelemetry-collector/blob/main/confmap/confmap.go#L351
be used as a type parameter to `Optional[T]`. The following config struct shows | ||
how this may look, both in definition and in usage: | ||
|
||
```golang |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More than golang code, would be good to show how users in yaml will use it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there's no expected impact on YAML
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bogdandrutu does Yuri's comment solve your question?
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
cc @open-telemetry/collector-approvers This has entered final comment period, I intend to merge this on Friday if there are no further blocking comments |
I believe none of the pending conversations are blocking, but I pinged the authors just in case. |
Description
This RFC will help us explore the use cases Optional types solve and move us toward a resolution for whether to adopt these in our config.
Related to #10266